Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[api-documenter] Add logic to disambiguate duplicate UID, file, or toc entries #1331

Closed
wants to merge 1 commit into from

Conversation

rbuckton
Copy link
Member

This PR adds logic to disambiguate duplicate UIDs, files, or toc entries.

This may require further iterations to come up with the best approach for synthesizing stable UIDs, however it currently uses the following rules:

UID/Path

  • UIDs and Paths are precomputed when the documenter is created. This is to ensure UIDs and Paths are computed stably across the entire model.
  • A UID for most items is a dot-qualified path (i.e. package.Namespace.ItemName)
  • A UID for a function-like includes the UIDs of its parameter list as a suffix:
  • If a UID remains ambiguous, a suffix of :<item kind> is appended if the item.
    • For a series of ambigouous items, the item with the highest precedence does not get a suffix:
      • Class, Enum (highest)
      • Interface, TypeAlias
      • Function, Variable
      • Namespace
      • Everything else
  • A Path is derived from the UID in this fashion <package part>/<non-package part><disambiguation>.yml
  • The Path will all be lowercase.
  • Unsafe path characters will be replaced with _.
  • A UID or Path is considered ambiguous if any preceding item has the same UID or Path.
  • If a UID or Path still remains ambiguous, a _<n> suffix is added where n is incremented until the UID is no longer ambiguous.

TOC Items

TOC items are disambiguated after creation by following the uid of the toc item back to its original item.

  • TOC items without a uid are not disambiguated.
  • TOC items with a name different from the name we would generate for the associated ApiItem are not disambiguated.
  • Disambiguated TOC items will always have a suffix appended based on the item kind (i.e. C (Class), C (Interface)).
  • If a TOC item is still ambiguous, a (<n>) suffix is added where n is incremented until the TOC item is no longer ambiguous.

Fixes #1308

@rbuckton
Copy link
Member Author

Note that this has a minor conflict with #1326 that would need to be resolved in either branch if merged.

- api-documenter-test.DocClass1.exampleFunction_1
- api-documenter-test.DocClass1.interestingEdgeCases
- api-documenter-test.DocClass1.deprecatedExample()
- 'api-documenter-test.DocClass1.exampleFunction(string,string)'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rbuckton What would you do with this input? 🙃

export declare function pathologicalExample(options: {
  args: {
    [name: string]: string | boolean;
  };
  buildErrorIconPath?: string;
  buildSuccessIconPath?: string;
  distFolder: string;
  gulp: GulpProxy | Gulp;
  isRedundantBuild?: boolean;
  jestEnabled?: boolean;
  libAMDFolder?: string;
  libES6Folder?: string;
  libESNextFolder?: string;
  libFolder: string;
  onTaskEnd?: (taskName: string, duration: number[], error?: any) => void;
  onTaskStart?: (taskName: string) => void;
  packageFolder: string;
  production: boolean;
  properties?: {
    [key: string]: any;
  };
  relogIssues?: boolean;
  rootPath: string;
  shouldWarningsFailBuild: boolean;
  showToast?: boolean;
  srcFolder: string;
  tempFolder: string;
  uniqueTasks?: IExecutable[];
  verbose: boolean;
} & Partial<ICopyConfig>): void;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

items:
- uid: package.pathologicalExample
  name: package.pathologicalExample
  type: function
  syntax:
    parameters:
    - id: options
      type:
      - anonymous0:local
    return:
      type:
      - void
references:
- uid: anonymous0:local
  name: |> <content of options type>
  spec.typeScript:
  - name: '{\n  args: {\n    [name: string]: string | boolean;\n  }; <snip> gulp: '
  - name: GulpProxy
    uid: otherpackage.GulpProxy
  - name: ' | '
  - name: Gulp
    uid: gulp.Gulp
  - name: ';\n  isRedundantBuild?: <snip> uniqueTasks?: '
  - name: IExecutable
    uid: otherpackage.IExecutable
  - name: '[];\n  verbose: boolean;\n} & '
  - name: Partial
    uid: Partial
  - name: '<'
  - name: ICopyConfig
    uid: otherpackage.ICopyConfig
  - name: '>'
- uid: otherpackage.GulpProxy
  name: GulpProxy
- uid: gulp.Gulp
  name: Gulp
- uid: otherpackage.IExecutable
  name: IExecutable
- uid: otherpackage.ICopyConfig
  name: ICopyConfig

You also could have a uid of {{args:{{[name:string]~string|boolean}},buildErrorIconPath?:string,buildSuccessIconPath?:string,...}&Partial{ICopyConfig} (with the ... replaced) since it doesn't need to be human readable, just machine readable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entry in references is effectively the same as an Excerpt, with some additional information (uid, nameWithType, fullName).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't need to be human readable, just machine readable.

@rbuckton From a technical perspective, this may be true. But I'm pretty uncomfortable about designing a system that emits huge unwieldy expressions in a file that people may have to understand/debug.

Also, what about the other related ID problems called out in #1308? We wouldn't want these huge expressions to appear in:

  • Markdown URLs
  • DocFX URLs
  • Titles that appear in member tables in the docs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Complex types should probably have aliased UIDs (possibly based on a hash of the stable UID). They would point to an entry in the yaml references list which would have the expanded text/excerpt that breaks down the type into hyperlinkable tokens.

@rbuckton
Copy link
Member Author

I plan to abandon this PR as I am truly starting to believe that Uid generation is better handled by api-extractor while it still has access to the TypeScript Program and TypeChecker. I'll follow up with a draft PR to show what I was thinking shortly.

@rbuckton
Copy link
Member Author

Here's the new approach I'm thinking about: https://github.com/rbuckton/web-build-tools/pull/1/files#diff-5bac56083efa60bf63727db8afb690e3

The UidGenerator class generates a stable UID from either a ts.Declaration, ts.Symbol, or ts.Type, even if the referenced symbol or type come form an external package.

It does generate ugly UIDs for complex types currently. I'm considering a mechanism for replacing what would be a very complex UID with a simpler reference UID like anonymous0:local, and adding uid to Excerpt and ExcerptToken as well. Then api-documenter would just need to record excerpts as references.

The upside of using a stable UID generation mechanism that is driven by the TS TypeChecker, is that two different projects that depend on the same third project will have the same UIDs generated for their references, so that if the documentation for all three is merged the referenced links will work appropriately.

@rbuckton
Copy link
Member Author

See https://gist.github.com/rbuckton/dcd5cf51b0a4d0fc7fe64405c1ec4edf for some additional thoughts I have.

Instead of {{args:{{[name:string]~string|boolean}},buildErrorIconPath?:string,buildSuccessIconPath?:string,...}&Partial{ICopyConfig}, you would end up with this instead:

items:
- uid: package.pathologicalExample(@0)
  name: package.pathologicalExample(@0)
  type: function
  syntax:
    parameters:
    - id: options
      type:
      - package.pathologicalExample@0
    return:
      type:
      - void
references:
- uid: package.pathologicalExample@0
  name: |> <content of options type>
  spec.typeScript:
  - name: '{\n  args: {\n    [name: string]: string | boolean;\n  }; <snip> gulp: '
  - name: GulpProxy
    uid: otherpackage.GulpProxy
  - name: ' | '
  - name: Gulp
    uid: gulp.Gulp
  - name: ';\n  isRedundantBuild?: <snip> uniqueTasks?: '
  - name: IExecutable
    uid: otherpackage.IExecutable
  - name: '[];\n  verbose: boolean;\n} & '
  - name: Partial
    uid: Partial
  - name: '<'
  - name: ICopyConfig
    uid: otherpackage.ICopyConfig
  - name: '>'
- uid: otherpackage.GulpProxy
  name: GulpProxy
- uid: gulp.Gulp
  name: Gulp
- uid: otherpackage.IExecutable
  name: IExecutable
- uid: otherpackage.ICopyConfig
  name: ICopyConfig

@octogonz
Copy link
Collaborator

@rbuckton should we close this PR now that #1337 is in?

@rbuckton
Copy link
Member Author

I need to revisit this, as #1337 only dealt with the uid generation part, I'm still concerned about diambiguating TOC items for cases like:

export interface Foo {}
export declare function Foo(): Foo;

Where the TOC might be generated as:

  • Foo
  • Foo

Its not so much an issue now as it will be if/when we fix how namespaces are added to the YAML (which also requires a DocFX fix).

@rbuckton
Copy link
Member Author

Closing in favor of #1536

@rbuckton rbuckton closed this Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[api-documenter] Fix ae-ambiguous-ids meta issue
2 participants